execution/commitment: remove the streaming commitment mode - #23191
execution/commitment: remove the streaming commitment mode#23191awskii wants to merge 7 commits into
Conversation
The streaming committer's background fold pool was never started in production: every StartScheduler caller was a test, so sc.base was always nil, sc.started always false, and TouchKey's enqueue branch never taken. --experimental.streaming-commitment advertised "overlaps folding with block execution" but folded synchronously, taking the same shape as the mounted parallel path through splits instead of mounts. Removes streaming_commitment.go, its tests, the StreamingCommitter wiring in ParallelPatriciaHashed and Updates, VariantStreamingHexPatricia, and the flag with its config plumbing. Kept, with live callers on the mounted parallel path: all of streaming_deep_fold.go (foldStorageRoot, dfsSubtreeDeep, unfoldStorageBase); keyArena/keyArenaChunk/touchedKey, moved there from streaming_commitment.go because collectSubtreeKeys needs them; stitchSplitCells, moved into parallel_mount.go. DeepLocalFolds moves to ParallelPatriciaHashed rather than disappearing with the committer. TestDeepFold_FreshWhaleFoldsParallel and TestDeepFold_ExistingWhaleStillDemotes assert on it to pin which fold path ran; without it they would degrade to root-parity checks that pass whichever path is taken. Mutation-checked: dropping the counter increment turns FreshWhale red.
…streaming removal
# Conflicts: # execution/commitment/deepfold_emptystorage_regression_test.go # execution/commitment/deepfold_regression_test.go # execution/commitment/deepfold_retouch_regression_test.go # execution/commitment/deepfold_singleslot_reexpand_regression_test.go # execution/commitment/deepfold_subset_regression_test.go # execution/commitment/streaming_commitment.go # execution/commitment/streaming_commitment_test.go
…ed deepfold tests The five deepfold_*_regression_test.go files this branch edited were merged into deepfold_test.go by #23184 (a pure move), so the mode-table and helper changes had to be reapplied there. keyArena keeps main's remaining-based sizing: moving the type out of the deleted streaming_commitment.go dropped the field, but its call site still passes it.
|
Merged main. Seven conflicts, all modify/delete, pointing both ways: Deleted here, modified on main — Deleted on main, modified here — the five One semantic break the textual merge hid: moving Verified on the pushed tree: |
There was a problem hiding this comment.
Pull request overview
This PR removes the unused “streaming commitment” variant from Erigon’s execution commitment pipeline (flag/config/plumbing, implementation, and tests), consolidating the surviving mounted-parallel commitment path and keeping the deep-fold helpers that are still used in production.
Changes:
- Removed the
StreamingCommitterimplementation, its variant (VariantStreamingHexPatricia), CLI/config flags, and related scheduled/streaming test modes. - Kept and refactored shared deep-fold logic for whale storage folding (moving
keyArena/touchedKeyand relocatingstitchSplitCells). - Added a
DeepLocalFoldscounter toParallelPatriciaHashedand updated tests to assert which fold path ran without relying on the deleted streaming engine.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| node/ethconfig/config.go | Removes the ExperimentalStreamingCommitment config field. |
| node/eth/backend.go | Drops propagation of the streaming commitment toggle into statecfg. |
| node/cli/default_flags.go | Removes the streaming-commitment CLI flag from defaults. |
| execution/commitment/streaming_deep_fold.go | Moves touchedKey/keyArena into the deep-fold helper file for continued use by the mounted path. |
| execution/commitment/streaming_commitment.go | Deletes the streaming committer implementation. |
| execution/commitment/streaming_commitment_test.go | Deletes streaming-committer tests. |
| execution/commitment/state_roundtrip_regression_test.go | Removes streaming variants from restart/state round-trip coverage. |
| execution/commitment/parallel_trace_test.go | Updates wording to remove streaming references. |
| execution/commitment/parallel_testkit_test.go | Removes streaming run modes; adds parallelBatchDeepFolds helper for fold-path assertions. |
| execution/commitment/parallel_streaming_bench_test.go | Removes streaming overlap benchmark code and related helpers. |
| execution/commitment/parallel_patricia_hashed.go | Removes streaming wiring; adds DeepLocalFolds counter to the parallel trie. |
| execution/commitment/parallel_patricia_hashed_test.go | Drops streaming variant from reuse-across-reset parity test. |
| execution/commitment/parallel_mount.go | Moves stitchSplitCells here; increments DeepLocalFolds on successful deep storage fold. |
| execution/commitment/mode_parallel_lifecycle_test.go | Removes streaming lifecycle assertions and variants. |
| execution/commitment/deepfold_test.go | Switches deep-fold assertions from streaming committer to parallel deep-fold counter. |
| execution/commitment/commitmentdb/commitment_context.go | Removes streaming references in variant selection comments and restore paths. |
| execution/commitment/commitment.go | Removes streaming variant and Updates→streaming funnel plumbing. |
| execution/commitment/additive_updates_test.go | Drops streaming additive-touch test case. |
| db/state/statecfg/state_schema.go | Removes the ExperimentalStreamingCommitment statecfg toggle. |
| db/state/squeeze.go | Removes streaming variant selection from commitment file rebuild path. |
| db/state/execctx/domain_shared.go | Simplifies trie variant selection to sequential vs parallel only. |
| db/state/execctx/commitment_flag_test.go | Removes streaming-flag tests and associated global-flag plumbing. |
| cmd/utils/flags.go | Removes --experimental.streaming-commitment flag and config wiring. |
| cmd/integration/commands/flags.go | Removes integration flag binding for streaming commitment. |
| cmd/evm/staterunner_test.go | Removes streaming flag handling in state runner test setup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // touchedKey is a snapshotted touch a background fold replays; hk is copied off | ||
| // the walk path while pk/upd reference the caller's stable backing. |
| for i, k := range keys { | ||
| ks := string(k) | ||
| ut.TouchPlainKey(ks, nil, func(c *KeyUpdate, _ []byte) { | ||
| c.plainKey = ks | ||
| c.hashedKey = KeyToHexNibbleHash(k) | ||
| c.update = &upds[i] | ||
| }) | ||
| } |
| // Reset clears the published root hash and resets the template so the instance | ||
| // can be reused; pooled workers stay cached for the next Process call. | ||
| func (p *ParallelPatriciaHashed) Reset() { | ||
| if p.template != nil { | ||
| p.template.Reset() | ||
| } | ||
| p.rootHash.Store(nil) | ||
| if p.streaming != nil { | ||
| p.streaming.Reset() | ||
| } | ||
| } |
keyArena outlived streaming_commitment.go — collectSubtreeKeys still uses it — but its two tests were in streaming_commitment_test.go and went with the file. They move to deepfold_test.go, next to the deep fold that drives them.
|
Correction to my merge: it dropped two live tests, now restored in
Found by diffing the sorted Test/Fuzz/Benchmark name sets across main, this branch, and the merge — the same check #23184 used. After the restore, the only names in main and not here are the streaming tests plus On the arena itself, since the merge kept main's
|
The streaming committer's background fold pool was never started in production — every one of the 12
StartSchedulercallers is a test, sosc.basewas always nil,sc.startedalways false, andTouchKey's enqueue branch never taken.--experimental.streaming-commitmentadvertised "overlaps folding with block execution" but folded synchronously, taking the same shape as the mounted parallel path through splits instead of mounts.Changes
Remove
streaming_commitment.goand its tests, theStreamingCommitterwiring inParallelPatriciaHashedandUpdates,VariantStreamingHexPatricia, and the flag with its config plumbing. 2552 deletions.Kept, each with a live caller on the surviving mounted parallel path:
streaming_deep_fold.go—foldStorageRoot,dfsSubtreeDeep,unfoldStorageBase, called fromprocessMountedkeyArena/keyArenaChunk/touchedKey— declared in the deleted file but needed bycollectSubtreeKeys, so moved intostreaming_deep_fold.gostitchSplitCells— moved intoparallel_mount.goDeepLocalFoldsmoves ontoParallelPatriciaHashedrather than disappearing with the committer.TestDeepFold_FreshWhaleFoldsParallelandTestDeepFold_ExistingWhaleStillDemotesuse it to assert which fold path ran; without it both degrade to root-parity checks that pass whichever path is taken. Mutation-checked — dropping the counter increment turns FreshWhale red.Conflicts with the keyArena sizing PR, which edits
keyArenain place; that one should land first.One caveat worth recording: during verification I saw a single unattributed test failure I could not reproduce in four subsequent full runs of
execution/commitment/...anddb/state/.... Worth watching CI rather than assuming it was noise.